-
Notifications
You must be signed in to change notification settings - Fork 280
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Step3] 경로 조회 리뷰 요청드립니다. #277
base: misudev
Are you sure you want to change the base?
Conversation
- 중복되는 코드 분리 (PathSteps)
- remove() 내 긴 로직 -> 메소드로 분리 - 좀 더 명확한 함수명으로 변경
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3단계 기능 구현 하시느라 고생 많으셨습니다!
이번 pr은 특별히 코멘트를 드릴 부분이 많이 없을 정도로 깔끔하게 잘 구현하셨네요 👍
그래서 도메인 객체의 재설계 상황에서 기존에 만들어 두었던 테스트를 활용해서
안전하게 리팩터링을 해볼 수 있는 경험을 드리고자 몇가지 코멘트를 남겨보았습니다!
코멘트를 참고해서 리팩터링 진행해보시면 좋을 것 같아요~
PathFinder pathFinder = PathFinder.from(lines); | ||
Path shortestPath = pathFinder.searchPath(source, target); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
경로를 찾기위해 lines라는 정보가 있고, 이 정보를 이용해 경로를 찾는데 이 경로를 찾는 책임을 PathFinder가 가지고 있고 그 결과 정보는 Path가 가지고 있네요!
객체지향에서 객체는 상태와 행위로 이루어져있다고 하는데요
경로를 찾기 위한 상태인 lines와 행위인 PathFinder를 하나의 객체로 도출해볼 수는 없을까요?
만약에 이렇게 리팩터링 할 경우 기존에 만들어 두었던 테스트를 활용해서 테스트의 검증이라는 보호 속에 리팩터링을 해보시는 것을 권해드립니다!
throw new IllegalArgumentException("합치려는 구간의 상행역이 하행역과 같아야 합니다."); | ||
} | ||
|
||
return Section.of(line, upStation, section.downStation, distance + section.distance); | ||
} | ||
|
||
|
||
public boolean isUpStation(Station station) { | ||
public boolean hasUpStationAs(Station station) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
변경된 네이밍이 의미를 조금 더 잘 전달하는 것 같아요 👍
@@ -34,23 +35,30 @@ public void add(Section section) { | |||
public void remove(Station station) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
적절한 사이즈로 메서드를 나누어 주셨네요 👍
public static List<Line> 노선_목록 = new ArrayList<>(); | ||
|
||
static { | ||
삼호선.addSection(남부터미널역, 양재역, 남부터미널역_양재역_거리); | ||
|
||
ReflectionTestUtils.setField(강남역, "id", 1L); | ||
ReflectionTestUtils.setField(교대역, "id", 2L); | ||
ReflectionTestUtils.setField(양재역, "id", 3L); | ||
ReflectionTestUtils.setField(남부터미널역, "id", 4L); | ||
|
||
노선_목록.addAll(Arrays.asList(이호선, 삼호선, 신분당선)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
픽스쳐 분리를 통해 테스트 코드 내 효과적인 중복 제거 👍
교대역 = 지하철역_생성_요청("교대역").jsonPath().getLong("id"); | ||
강남역 = 지하철역_생성_요청("강남역").jsonPath().getLong("id"); | ||
양재역 = 지하철역_생성_요청("양재역").jsonPath().getLong("id"); | ||
남부터미널역 = 지하철역_생성_요청("남부터미널역").jsonPath().getLong("id"); | ||
청량리역 = 지하철역_생성_요청("청량리역").jsonPath().getLong("id"); | ||
회기역 = 지하철역_생성_요청("회기역").jsonPath().getLong("id"); | ||
|
||
일호선 = 지하철_노선_생성_요청("1호선", "blue", 청량리역, 회기역, 청량리역_회기역_거리).jsonPath().getLong("id"); | ||
이호선 = 지하철_노선_생성_요청("2호선", "green", 교대역, 강남역, 교대역_강남역_거리).jsonPath().getLong("id"); | ||
신분당선 = 지하철_노선_생성_요청("신분당선", "red", 강남역, 양재역, 강남역_양재역_거리).jsonPath().getLong("id"); | ||
삼호선 = 지하철_노선_생성_요청("3호선", "orange", 교대역, 남부터미널역, 교대역_남부터미널역_거리).jsonPath().getLong("id"); | ||
|
||
지하철_노선에_지하철_구간_생성_요청(삼호선, createSectionCreateParams(남부터미널역, 양재역, 남부터미널역_양재역_거리)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
단위 테스트의 픽스쳐 관리 방법처럼 해당 객체들의 재사용이 필요한 경우 인수 테스트용 픽스쳐를 만들어서 관리할 수 도 있겠네요!
안녕하세요 성현님~3단계 리뷰 요청드립니다.
매번 꼼꼼하게 피드백 주셔서 감사합니다!
마지막도 잘 부탁드립니다 🙂